From 55bfac8dfd0bde5dcf3e1f88ed5ed7a1bc9a9e4d Mon Sep 17 00:00:00 2001 From: "kfraser@localhost.localdomain" Date: Wed, 23 Aug 2006 18:38:08 +0100 Subject: [PATCH] [XEN] Fix the x86 emulator to safely fail when it turns out the faulting memory access was to/from an implicit memory operand (typically either an instruction fetch or stack access). Rationalise use of macros for page fault error code flags. This patch supercedes the fix in changeset 11242. Signed-off-by: Keir Fraser --- tools/tests/test_x86_emulator.c | 14 ++++++++++++ xen/arch/x86/shadow2.c | 10 ++++----- xen/arch/x86/traps.c | 16 ++++++------- xen/arch/x86/x86_emulate.c | 35 ++++++++++++++++++++++++----- xen/include/asm-x86/processor.h | 10 ++++----- xen/include/asm-x86/shadow2-types.h | 13 ----------- 6 files changed, 62 insertions(+), 36 deletions(-) diff --git a/tools/tests/test_x86_emulator.c b/tools/tests/test_x86_emulator.c index 11755c3373..d3b7af0a7c 100644 --- a/tools/tests/test_x86_emulator.c +++ b/tools/tests/test_x86_emulator.c @@ -15,6 +15,8 @@ typedef int64_t s64; #include #include +#define PFEC_write_access (1U<<1) + static int read_any( unsigned long addr, unsigned long *val, @@ -105,6 +107,7 @@ int main(int argc, char **argv) regs.eflags = 0x200; regs.eip = (unsigned long)&instr[0]; regs.ecx = 0x12345678; + regs.error_code = PFEC_write_access; ctxt.cr2 = (unsigned long)res; *res = 0x7FFFFFFF; rc = x86_emulate_memop(&ctxt, &emulops); @@ -125,6 +128,7 @@ int main(int argc, char **argv) regs.ecx = 0x12345678UL; #endif ctxt.cr2 = (unsigned long)res; + regs.error_code = 0; rc = x86_emulate_memop(&ctxt, &emulops); if ( (rc != 0) || (*res != 0x92345677) || @@ -139,6 +143,7 @@ int main(int argc, char **argv) regs.eip = (unsigned long)&instr[0]; regs.ecx = ~0UL; ctxt.cr2 = (unsigned long)res; + regs.error_code = 0; rc = x86_emulate_memop(&ctxt, &emulops); if ( (rc != 0) || (*res != 0x92345677) || @@ -154,6 +159,7 @@ int main(int argc, char **argv) regs.eax = 0x92345677UL; regs.ecx = 0xAA; ctxt.cr2 = (unsigned long)res; + regs.error_code = PFEC_write_access; rc = x86_emulate_memop(&ctxt, &emulops); if ( (rc != 0) || (*res != 0x923456AA) || @@ -170,6 +176,7 @@ int main(int argc, char **argv) regs.eax = 0xAABBCC77UL; regs.ecx = 0xFF; ctxt.cr2 = (unsigned long)res; + regs.error_code = PFEC_write_access; rc = x86_emulate_memop(&ctxt, &emulops); if ( (rc != 0) || (*res != 0x923456AA) || @@ -186,6 +193,7 @@ int main(int argc, char **argv) regs.eip = (unsigned long)&instr[0]; regs.ecx = 0x12345678; ctxt.cr2 = (unsigned long)res; + regs.error_code = PFEC_write_access; rc = x86_emulate_memop(&ctxt, &emulops); if ( (rc != 0) || (*res != 0x12345678) || @@ -203,6 +211,7 @@ int main(int argc, char **argv) regs.eax = 0x923456AAUL; regs.ecx = 0xDDEEFF00L; ctxt.cr2 = (unsigned long)res; + regs.error_code = PFEC_write_access; rc = x86_emulate_memop(&ctxt, &emulops); if ( (rc != 0) || (*res != 0xDDEEFF00) || @@ -240,6 +249,7 @@ int main(int argc, char **argv) regs.eip = (unsigned long)&instr[0]; regs.edi = (unsigned long)res; ctxt.cr2 = regs.edi; + regs.error_code = PFEC_write_access; rc = x86_emulate_memop(&ctxt, &emulops); if ( (rc != 0) || (*res != 0x2233445D) || @@ -261,6 +271,7 @@ int main(int argc, char **argv) regs.eip = (unsigned long)&instr[0]; regs.edi = (unsigned long)res; ctxt.cr2 = regs.edi; + regs.error_code = PFEC_write_access; rc = x86_emulate_memop(&ctxt, &emulops); if ( (rc != 0) || (res[0] != 0x9999AAAA) || @@ -275,6 +286,7 @@ int main(int argc, char **argv) regs.eip = (unsigned long)&instr[0]; regs.edi = (unsigned long)res; ctxt.cr2 = regs.edi; + regs.error_code = PFEC_write_access; rc = x86_emulate_memop(&ctxt, &emulops); if ( (rc != 0) || (res[0] != 0x9999AAAA) || @@ -292,6 +304,7 @@ int main(int argc, char **argv) regs.ecx = 0x12345678; ctxt.cr2 = (unsigned long)res; *res = 0x82; + regs.error_code = 0; rc = x86_emulate_memop(&ctxt, &emulops); if ( (rc != 0) || (*res != 0x82) || @@ -307,6 +320,7 @@ int main(int argc, char **argv) regs.ecx = 0x12345678; ctxt.cr2 = (unsigned long)res; *res = 0x1234aa82; + regs.error_code = 0; rc = x86_emulate_memop(&ctxt, &emulops); if ( (rc != 0) || (*res != 0x1234aa82) || diff --git a/xen/arch/x86/shadow2.c b/xen/arch/x86/shadow2.c index f5278c4a0e..943091b716 100644 --- a/xen/arch/x86/shadow2.c +++ b/xen/arch/x86/shadow2.c @@ -2893,7 +2893,7 @@ static int sh2_page_fault(struct vcpu *v, // i.e. ring 3. Such errors are not caused or dealt with by the shadow // code. // - if ( (regs->error_code & X86_PFEC_SUPERVISOR_FAULT) && + if ( (regs->error_code & PFEC_user_mode) && !(accumulated_gflags & _PAGE_USER) ) { /* illegal user-mode access to supervisor-only page */ @@ -2903,7 +2903,7 @@ static int sh2_page_fault(struct vcpu *v, // Was it a write fault? // - if ( regs->error_code & X86_PFEC_WRITE_FAULT ) + if ( regs->error_code & PFEC_write_access ) { if ( unlikely(!(accumulated_gflags & _PAGE_RW)) ) { @@ -2917,7 +2917,7 @@ static int sh2_page_fault(struct vcpu *v, // marked "do not execute". Such errors are not caused or dealt with // by the shadow code. // - if ( regs->error_code & X86_PFEC_INSN_FETCH_FAULT ) + if ( regs->error_code & PFEC_insn_fetch ) { if ( accumulated_gflags & _PAGE_NX_BIT ) { @@ -2960,8 +2960,8 @@ static int sh2_page_fault(struct vcpu *v, * for the shadow entry, since we might promote a page here. */ // XXX -- this code will need to change somewhat if/when the shadow code // can directly map superpages... - ft = ((regs->error_code & X86_PFEC_WRITE_FAULT) - ? ft_demand_write : ft_demand_read); + ft = ((regs->error_code & PFEC_write_access) ? + ft_demand_write : ft_demand_read); ptr_sl1e = shadow_get_and_create_l1e(v, &gw, &sl1mfn, ft); ASSERT(ptr_sl1e); diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index bc756699ee..f47a682297 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -686,9 +686,9 @@ void propagate_page_fault(unsigned long addr, u16 error_code) v->vcpu_info->arch.cr2 = addr; /* Re-set error_code.user flag appropriately for the guest. */ - error_code &= ~PGERR_user_mode; + error_code &= ~PFEC_user_mode; if ( !guest_kernel_mode(v, guest_cpu_user_regs()) ) - error_code |= PGERR_user_mode; + error_code |= PFEC_user_mode; ti = &v->arch.guest_context.trap_ctxt[TRAP_page_fault]; tb->flags = TBF_EXCEPTION | TBF_EXCEPTION_ERRCODE; @@ -768,17 +768,17 @@ static int __spurious_page_fault( unsigned int required_flags, disallowed_flags; /* Reserved bit violations are never spurious faults. */ - if ( regs->error_code & PGERR_reserved_bit ) + if ( regs->error_code & PFEC_reserved_bit ) return 0; required_flags = _PAGE_PRESENT; - if ( regs->error_code & PGERR_write_access ) + if ( regs->error_code & PFEC_write_access ) required_flags |= _PAGE_RW; - if ( regs->error_code & PGERR_user_mode ) + if ( regs->error_code & PFEC_user_mode ) required_flags |= _PAGE_USER; disallowed_flags = 0; - if ( regs->error_code & PGERR_instr_fetch ) + if ( regs->error_code & PFEC_insn_fetch ) disallowed_flags |= _PAGE_NX; mfn = cr3 >> PAGE_SHIFT; @@ -886,7 +886,7 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs) guest_kernel_mode(v, regs) && /* Do not check if access-protection fault since the page may legitimately be not present in shadow page tables */ - ((regs->error_code & PGERR_write_access) == PGERR_write_access) && + ((regs->error_code & PFEC_write_access) == PFEC_write_access) && ptwr_do_page_fault(d, addr, regs) ) return EXCRET_fault_fixed; @@ -1100,7 +1100,7 @@ static int emulate_privileged_op(struct cpu_user_regs *regs) if ( (rc = copy_to_user((void *)regs->edi, &data, op_bytes)) != 0 ) { propagate_page_fault(regs->edi + op_bytes - rc, - PGERR_write_access); + PFEC_write_access); return EXCRET_fault_fixed; } regs->edi += (int)((regs->eflags & EF_DF) ? -op_bytes : op_bytes); diff --git a/xen/arch/x86/x86_emulate.c b/xen/arch/x86/x86_emulate.c index 4016aa77e3..e7f1a77196 100644 --- a/xen/arch/x86/x86_emulate.c +++ b/xen/arch/x86/x86_emulate.c @@ -21,6 +21,11 @@ #endif #include +#ifndef PFEC_write_access +#define PFEC_write_access (1U<<1) +#define PFEC_insn_fetch (1U<<4) +#endif + /* * Opcode effective-address decode tables. * Note that we only emulate instructions that have at least one memory @@ -444,6 +449,13 @@ x86_emulate_memop( /* Shadow copy of register state. Committed on successful emulation. */ struct cpu_user_regs _regs = *ctxt->regs; + /* + * We do not emulate faults on instruction fetch. We assume that the + * guest never executes out of a special memory area. + */ + if ( _regs.error_code & PFEC_insn_fetch ) + return -1; + switch ( mode ) { case X86EMUL_MODE_REAL: @@ -620,6 +632,14 @@ x86_emulate_memop( } break; case DstMem: + /* + * We expect that the fault occurred while accessing the explicit + * destination memory operand. This is clearly not the case if the + * fault occurred on a read access (eg. POP has an *implicit* operand + * but we expect that the guest never uses special memory as stack). + */ + if ( !(_regs.error_code & PFEC_write_access) ) + goto cannot_emulate; dst.type = OP_MEM; dst.ptr = (unsigned long *)cr2; dst.bytes = (d & ByteOp) ? 1 : op_bytes; @@ -664,6 +684,14 @@ x86_emulate_memop( case SrcMem: src.bytes = (d & ByteOp) ? 1 : op_bytes; srcmem_common: + /* + * We expect that the fault occurred while accessing the explicit + * source memory operand. This is clearly not the case if the fault + * occurred on a write access (eg. PUSH has an *implicit* operand + * but we expect that the guest never uses special memory as stack). + */ + if ( _regs.error_code & PFEC_write_access ) + goto cannot_emulate; src.type = OP_MEM; src.ptr = (unsigned long *)cr2; if ( (rc = ops->read_emulated((unsigned long)src.ptr, @@ -846,9 +874,6 @@ x86_emulate_memop( emulate_1op("dec", dst, _regs.eflags); break; case 6: /* push */ - /* Don't emulate if fault was on stack */ - if ( _regs.error_code & 2 ) - goto cannot_emulate; /* 64-bit mode: PUSH always pushes a 64-bit operand. */ if ( mode == X86EMUL_MODE_PROT64 ) { @@ -923,7 +948,7 @@ x86_emulate_memop( case 0xa4 ... 0xa5: /* movs */ dst.type = OP_MEM; dst.bytes = (d & ByteOp) ? 1 : op_bytes; - if ( _regs.error_code & 2 ) + if ( _regs.error_code & PFEC_write_access ) { /* Write fault: destination is special memory. */ dst.ptr = (unsigned long *)cr2; @@ -1165,7 +1190,7 @@ x86_emulate_write_std( if ( (rc = copy_to_user((void *)addr, (void *)&val, bytes)) != 0 ) { - propagate_page_fault(addr + bytes - rc, PGERR_write_access); + propagate_page_fault(addr + bytes - rc, PFEC_write_access); return X86EMUL_PROPAGATE_FAULT; } diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h index 81c8757f8e..196a5bc5bc 100644 --- a/xen/include/asm-x86/processor.h +++ b/xen/include/asm-x86/processor.h @@ -130,11 +130,11 @@ #define TF_kernel_mode (1<<_TF_kernel_mode) /* #PF error code values. */ -#define PGERR_page_present (1U<<0) -#define PGERR_write_access (1U<<1) -#define PGERR_user_mode (1U<<2) -#define PGERR_reserved_bit (1U<<3) -#define PGERR_instr_fetch (1U<<4) +#define PFEC_page_present (1U<<0) +#define PFEC_write_access (1U<<1) +#define PFEC_user_mode (1U<<2) +#define PFEC_reserved_bit (1U<<3) +#define PFEC_insn_fetch (1U<<4) #ifndef __ASSEMBLY__ diff --git a/xen/include/asm-x86/shadow2-types.h b/xen/include/asm-x86/shadow2-types.h index fcc6b466f8..13107f4566 100644 --- a/xen/include/asm-x86/shadow2-types.h +++ b/xen/include/asm-x86/shadow2-types.h @@ -461,19 +461,6 @@ struct shadow2_walk_t mfn_t l1mfn; /* MFN that the level 1 entry is in */ }; - -/* X86 error code bits: - * These bits certainly ought to be defined somewhere other than here, - * but until that place is determined, here they sit. - * - * "PFEC" == "Page Fault Error Code" - */ -#define X86_PFEC_PRESENT 1 /* 0 == page was not present */ -#define X86_PFEC_WRITE_FAULT 2 /* 0 == reading, 1 == writing */ -#define X86_PFEC_SUPERVISOR_FAULT 4 /* 0 == supervisor-mode, 1 == user */ -#define X86_PFEC_RESERVED_BIT_FAULT 8 /* 1 == reserved bits set in pte */ -#define X86_PFEC_INSN_FETCH_FAULT 16 /* 0 == normal, 1 == instr'n fetch */ - /* macros for dealing with the naming of the internal function names of the * shadow code's external entry points. */ -- 2.30.2